Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix 271 #288

Merged
merged 7 commits into from
Aug 8, 2022
Merged

Fix 271 #288

merged 7 commits into from
Aug 8, 2022

Conversation

zenhack
Copy link
Contributor

@zenhack zenhack commented Aug 8, 2022

The actual fix is the last commit; see the commit message.

There are also a couple other improvements in this pr that I made along the way:

  1. Just above the fix for Method call with expired context causes nil-pointer panic #271, there was a spot where we were mutating the questions table without holding c.mu. I have not observed symptoms from this, but this is wrong so I added a syncutil.With around it while I was there.
  2. The panic message for this failure mode is now more informative; it tells you which method was called originally, and if it was reject, what the error was.

Note, this fixed both tests; I can't reproduce #272. @lthibault, are you still able to reproduce it on this branch?

lthibault and others added 7 commits July 28, 2022 19:45
...after merging in main, this no longer built, due to the change
removing struct wrappers.
The callback is run without holding c.mu, so we need to acquire it to
mutate the questions table.

I have not (to my knowledge) observed symptoms of this, but spotted it
while debugging something else.
Silly mistake: when a pipelined question failed to send, we were
rejecting the promise for the *original* question, not the new one we
just failed to send -- so when the return message for the original came
back, we got a panic. Solution is to reject q2, not q.

Note that the tests also needed tweaking, because we don't expect no
error, we just don't want a panic -- the error should reflect the
canceled context.
@zenhack zenhack requested a review from lthibault August 8, 2022 21:29
@zenhack
Copy link
Contributor Author

zenhack commented Aug 8, 2022

While debugging this, I added some code to record a stack trace for the first call to Fulfill/Reject, which was helpful. I didn't keep it in because it means keeping a buffer around for each fulfilled promise, and the way I did it I just oversized it so it would be 1MiB per promise, but worth bearing in mind/maybe thinking about how we can make the system easier to introspect for stuff like this.

Copy link
Collaborator

@lthibault lthibault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was quick! 😃

I'll run the tests on my machine after merging.

@lthibault lthibault merged commit e667e0d into capnproto:main Aug 8, 2022
@zenhack zenhack deleted the fix-271 branch August 8, 2022 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants